-
Notifications
You must be signed in to change notification settings - Fork 68
Fix (Global Colors): fix dragging in color picker #3590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughForwarded refs were added to SortablePicker by converting it to forwardRef and attaching the ref to its root element. The color picker integration was refactored by extracting a dedicated ItemPickerColor component and wiring ColorPickers to use it, preserving value updates via onChange with color merging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant SP as SortablePicker (forwardRef)
participant IPC as ItemPickerColor
participant CP as ColorPicker
participant P as Parent (Global Colors UI)
U->>SP: Open item editor
SP->>IPC: Render ItemPicker (with item, onChange)
IPC->>CP: Render ColorPicker(color=item.color, enableAlpha=true)
U->>CP: Drag/select color
CP-->>IPC: onChange(value)
IPC-->>SP: onChange({ ...item, color: value })
SP-->>P: Propagate updated items
note over SP,P: SP exposes root DOM via forwarded ref (outside-click/drag handling)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/global-settings/colors/color-picker.js (1)
128-132: Auto-open of the newly added color likely no-ops due to missingforwardRefin SortablePicker.
refpassed to<SortablePicker ref={ ref } />won’t reach the inner<div ref={ ref }>withoutforwardRef. As a result,ref.current?.querySelector(...).click()won’t run. See proposedforwardReffix insrc/components/sortable-picker/index.js.
🧹 Nitpick comments (2)
src/plugins/global-settings/colors/color-picker.js (1)
27-40: Good fix: exporting the picker component restores drag behavior. Also keeprgbin sync.LGTM on exporting
ItemPickerColorto avoid the drag regression. However, updates only setitem.colorand leaveitem.rgbstale. If other UI relies onrgb, this will desync on drag.Apply:
export const ItemPickerColor = ( { item, onChange } ) => { return <div className="stk-color-palette-control__popover-content"> <ColorPicker - onChange={ value => onChange( { - ...item, - color: value, - } ) } + onChange={ value => onChange( { + ...item, + color: value, + rgb: getRgb( value ), + } ) } color={ item.color } enableAlpha={ true } /> </div> }src/components/sortable-picker/index.js (1)
23-23: Decouple from plugin-specific registry.Importing
ITEM_PICKERhere ties components to plugin code (via util). Prefer a resolver prop with a safe fallback.Apply:
-import { ITEM_PICKER } from './util' +import { ITEM_PICKER } from './util' // keep for backward compat, but prefer props.itemPickerResolverThen in renderContent (see next comment) use
props.itemPickerResolver || ITEM_PICKER.
Also add a default prop:SortablePicker.defaultProps = { className: '', label: '', onReset: () => {}, + itemPickerResolver: {}, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/sortable-picker/index.js(2 hunks)src/components/sortable-picker/util.js(1 hunks)src/plugins/global-settings/colors/color-picker.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/sortable-picker/util.js (1)
src/plugins/global-settings/colors/color-picker.js (2)
ItemPickerColor(29-40)ItemPickerColor(29-40)
src/components/sortable-picker/index.js (1)
src/components/sortable-picker/util.js (2)
ITEM_PICKER(3-5)ITEM_PICKER(3-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/sortable-picker/index.js (3)
48-61: Ref‑forwarding conversion looks good.Signature switched to forwardRef((props, ref)) with correct prop destructuring. No behavior change otherwise.
Apply a tiny DX tweak so the component shows a readable name in React DevTools:
} ) +SortablePicker.displayName = 'SortablePicker' + SortablePicker.defaultProps = {
72-85: Guard against null AddItemPopover to avoid runtime errors when dropdownOnAdd is true.If
AddItemPopoveris null and someone toggles the dropdown (e.g., via keyboard),<AddItemPopover .../>will throw.Suggested change:
- { enableAddItem && <Dropdown + { enableAddItem && ( !dropdownOnAdd || AddItemPopover ) && <Dropdown popoverProps={ addItemPopoverProps } renderToggle={ ( { onToggle, isOpen } ) => ( <Button className="ugb-global-settings-color-picker__add-button" onClick={ dropdownOnAdd ? onToggle : handleAddItem } icon="plus-alt2" aria-expanded={ isOpen } /> ) } - renderContent={ ( { onClose } ) => ( - <AddItemPopover onClose={ onClose } onChange={ handleAddItem } /> - ) } + renderContent={ ( { onClose } ) => + AddItemPopover ? ( + <AddItemPopover onClose={ onClose } onChange={ handleAddItem } /> + ) : null + } /> }
119-133: Prefer stable keys over array indices.Using
indexaskeywill cause unnecessary remounts during reordering. Use a stable identifier (e.g.,item.slug) if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/sortable-picker/index.js(3 hunks)src/plugins/global-settings/colors/color-picker.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/global-settings/colors/color-picker.js (1)
src/components/color-palette-control/index.js (2)
value(162-162)onChange(163-163)
src/components/sortable-picker/index.js (2)
src/plugins/global-settings/colors/color-picker.js (1)
ref(41-41)src/plugins/global-settings/color-schemes/color-scheme-picker.js (1)
ref(86-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.5.5
🔇 Additional comments (4)
src/components/sortable-picker/index.js (2)
20-22: Importing forwardRef is correct and necessary.Good addition to enable ref forwarding.
86-93: Attaching the forwarded ref to the container is appropriate.This exposes the correct subtree for the “open newly added color” click via querySelector.
Manual check:
- Add a new color; confirm the picker opens automatically.
- Ensure ref.current is a DOM node and querySelector finds the last .stk-global-settings-color-picker__color-indicator-wrapper dropdown.
src/plugins/global-settings/colors/color-picker.js (2)
27-38: ItemPickerColor wiring is correct; uses ColorPicker’s string onChange API.
@wordpress/componentsColorPicker fires onChange with a hex/hex8 string; storing it initem.coloris appropriate. Consider accepting and passing throughonClosefor future needs (e.g., committing on close), but not required here.Quick UX check:
- Drag inside the gradient and alpha slider; color updates continuously without losing focus.
- Press Esc; popover closes (Dropdown handles it).
160-160: Passing ItemPickerColor directly simplifies integration and fixes drag interactions.This avoids dynamic string resolution and ensures the picker remains interactive for click‑and‑drag.
Acceptance criteria to verify against Issue #3575:
- You can click‑and‑drag inside the color gradient to update the color.
- Clicking anywhere outside the picker closes the panel automatically.
If either fails, ping and I’ll help trace the event handling.
fixes #3575
Summary by CodeRabbit